Adds overloads to ak.full #5223
Conversation
drculhane
force-pushed
the
Closes-5173
branch
from
ff4a621 to
6f1020e
Compare
drculhane
force-pushed
the
Closes-5173
branch
from
6a7ec72 to
43efbc2
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5223 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 4
Lines ? 63
Branches ? 0
========================================
Hits ? 63
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
drculhane
force-pushed
the
Closes-5173
branch
from
43efbc2 to
5c5a71b
Compare
arkouda/numpy/pdarraycreation.py
Outdated
| SizeArg = Union[int_scalars, Tuple[int_scalars, ...], str] | ||
| NumericFill = Union[numeric_scalars, np.bool] | ||
| FillValue = Union[NumericFill, str] | ||
| NumericDTypeArg = Union[np.dtype, type, bigint] |
There was a problem hiding this comment.
Can you use the type aliases in arkouda.numpy._typing if possible? If not, then add the new definitions there?
There was a problem hiding this comment.
I've removed these aliases. They were meant to make the code clearer, but I don't think they were even doing that.
arkouda/numpy/pdarraycreation.py
Outdated
| @overload | ||
| def full( | ||
| size: SizeArg, | ||
| fill_value: str, |
There was a problem hiding this comment.
I would recommend using StringDTypeTypes instead of str, in case someone uses a np.str_.
Same for the other ones.
There was a problem hiding this comment.
That breaks these overloads substantially.
Would you consider making this a separate issue? This file doesn't import StringDTypeTypes at all, and there are multiple functions that accept an argument of type "str".
arkouda/numpy/pdarraycreation.py
Outdated
| size: int_scalars or tuple of int_scalars | ||
| Size or shape of the array | ||
| fill_value: int_scalars or str | ||
| fill_value: numeric_scalars or str |
There was a problem hiding this comment.
This should include str_scalars.
There was a problem hiding this comment.
Done. Also changed fill_value to allow bool_scalars instead of just np.bool_.
There was a problem hiding this comment.
Thanks. It should be changed in the signature as well.
| raise ValueError(f"Size parameter {size} incompatible with Strings.") | ||
| else: | ||
| if isinstance(fill_value, float): # needed to match numpy, which | ||
| fill_value = int(fill_value) # truncates floats to ints before "str" |
There was a problem hiding this comment.
This line leads to this error:
In [8]: ak.full(n, np.nan, dtype=ak.str_)
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
Cell In[8], line 1
----> 1 ak.full(n, np.nan, dtype=ak.str_)
File ~/anaconda3/envs/arkouda-dev/lib/python3.13/site-packages/typeguard/__init__.py:891, in typechecked.<locals>.wrapper(*args, **kwargs)
889 memo = _CallMemo(python_func, _localns, args=args, kwargs=kwargs)
890 check_argument_types(memo)
--> 891 retval = func(*args, **kwargs)
892 check_return_type(retval, memo)
894 # If a generator is returned, wrap it if its yield/send/return types can be checked
File ~/git/arkouda/arkouda/numpy/pdarraycreation.py:882, in full(size, fill_value, dtype, max_bits)
880 else:
881 if isinstance(fill_value, float): # needed to match numpy, which
--> 882 fill_value = int(fill_value) # truncates floats to ints before "str"
883 return _full_string(full_size, str(fill_value))
885 # Remaining cases are all numeric. Check dtype for error
ValueError: cannot convert float NaN to integer
Also consider what happens for np.inf and -np.inf.
There was a problem hiding this comment.
Those were interesting edge cases. Handled now, with ifs.
3 participants
Closes #5173
This one required a fair amount of work to satisfy mypy once the overloads were written. I'm going to keep it in draft initially.
Notes:
In addition to the work on the overloads, mypy found an error in the order of calling parameters to full in my recent PR that changed ak.minimum and ak.maximum. So that's been fixed.
Once the overloads were in place, mypy flagged an error in the use of full in isna function in the recent pandas extensions. I addressed that by converting the result of ak.full to an ArkoudaArray before returning.